Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

billing errors and warnings, stripe and orb webhook, river worker for async background jobs #5440

Merged
merged 37 commits into from
Sep 19, 2024

Conversation

pjain1
Copy link
Member

@pjain1 pjain1 commented Aug 13, 2024

@pjain1 pjain1 marked this pull request as draft August 13, 2024 10:19
@pjain1 pjain1 changed the title stripe webhook, river worker for async background jobs stripe and orb webhook, river worker for async background jobs Aug 23, 2024
@pjain1 pjain1 marked this pull request as ready for review September 4, 2024 18:17
admin/server/server.go Outdated Show resolved Hide resolved
@pjain1 pjain1 changed the title stripe and orb webhook, river worker for async background jobs billing errors and warnings, stripe and orb webhook, river worker for async background jobs Sep 6, 2024
@pjain1 pjain1 self-assigned this Sep 6, 2024
admin/billing/biller.go Outdated Show resolved Hide resolved
admin/billing/biller.go Outdated Show resolved Hide resolved
admin/billing/payment/payment.go Outdated Show resolved Hide resolved
admin/billing/payment/stripe_webhook.go Outdated Show resolved Hide resolved
admin/billing/payment/stripe_webhook.go Outdated Show resolved Hide resolved
proto/rill/admin/v1/api.proto Outdated Show resolved Hide resolved
proto/rill/admin/v1/api.proto Outdated Show resolved Hide resolved
proto/rill/admin/v1/api.proto Outdated Show resolved Hide resolved
proto/rill/admin/v1/api.proto Outdated Show resolved Hide resolved
Comment on lines 2369 to 2402
message BillingErrorMetadata {
oneof metadata {
BillingErrorMetadataNoPaymentMethod no_payment_method = 1;
BillingErrorMetadataNoBillableAddress no_billable_address = 2;
BillingErrorMetadataInvoicePaymentFailed invoice_payment_failed = 3;
BillingErrorMetadataTrialEnded trial_ended = 4;
BillingErrorMetadataSubscriptionCancelled subscription_cancelled = 5;
}
}

message BillingErrorMetadataNoPaymentMethod {}

message BillingErrorMetadataNoBillableAddress {}

message BillingErrorMetadataInvoicePaymentFailed {
repeated InvoicePaymentFailedMeta invoices = 1;
}

message InvoicePaymentFailedMeta {
string invoice_id = 1;
string invoice_number = 2;
string invoice_url = 3;
string amount_due = 4;
string currency = 5;
google.protobuf.Timestamp due_date = 6;
}

message BillingErrorMetadataTrialEnded {
google.protobuf.Timestamp grace_period_end_date = 1;
}

message BillingErrorMetadataSubscriptionCancelled {
google.protobuf.Timestamp end_date = 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering from a simplicity point of view whether this info could be simplified to a string message? Or does the CLI/frontend need it destructured like this?

Copy link
Member Author

@pjain1 pjain1 Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier I had string message but found that this to be would be better for UI if they want to create call to action. For example failed invoice meta contains url that can directly take them to the failed invoice page and they can pay. Or use some of the end dates to perform something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a CTA, couldn't that be managed with either just an error code and message, or message and a cta_url? Or does the frontend need deeper understanding of the error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still feel its better to have it like this. UI can choose to create their own message. Another example on how these fields can be used is trial ending soon metadata contains trial end date or sub cancel error metadata contains sub end date so the UI can choose to display some banner or pop up on the end date or sometime before end date etc. Don't think it will be complicated for UI to use these to create flows as per product requirement since its typed metadata instead of json blobs.

admin/database/database.go Outdated Show resolved Hide resolved
admin/database/database.go Outdated Show resolved Hide resolved
admin/jobs/river/biller_event_handlers.go Outdated Show resolved Hide resolved
proto/rill/admin/v1/api.proto Outdated Show resolved Hide resolved
Comment on lines +133 to +135
if s.webhookSecret == "" {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it a panic risk to mount a nil handler? Maybe safer to return a handler that does nothing

Copy link
Member Author

@pjain1 pjain1 Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// WebhookHandlerFunc returns a http.HandlerFunc that can be used to handle incoming webhooks from the payment provider. Return nil if you don't want to register any webhook handlers.

Its documented and returning a handler that just does nothing returns nil, would send http ok and webhook events will be just lost. Instead a handler like this needs to be mounted

return func(w http.ResponseWriter, r *http.Request) error {
    return httputil.Errorf(http.StatusServiceUnavailable, "webhook secret not set")
}

Let me know if this should be done

InvoiceID: invoiceID,
GracePeriodEndDate: gracePeriodEndDate,
}, &river.InsertOpts{
ScheduledAt: gracePeriodEndDate.AddDate(0, 0, 1).Add(1 * time.Hour), // end of grace period date + 1 hour buffer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I think we need to reassess the use of ScheduledAt. Having jobs scheduled far in the future makes them much harder to maintain because you have to think about backwards compatibility and job migrations. There's also issues of jobs going stale that needs to be cancelled or correctly ignored upon invocation.

For example, if you look at Temporal, which is a more advanced background job system, you'll see they devote a lot of docs to discussing versioning and compatibility issues for these kinds of issues, and have some advanced features for dealing with them.

Looking at the use of ScheduledAt, it seems like these cases could relatively easily be moved into a cron job? For example, for this InvoicePaymentFailedGracePeriodCheck check, you could have an payment_failed_grace_period_ends_on timestamp in the orgs table, and have a cron job that grabs all rows where it is in the past and processes them. It is effectively the same approach, but makes the job itself stateless and easy to change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I see, makes sense. May be we can have cron jobs for all the billing errors/warning that requires some processing in future. When they wake up (mostly past every midnight) they look for all the orgs with that billing error and checks if its action time and takes action. We will need to maintain a flag if the billing error is processed or not to prevent reprocessing.

The problem you mentioned about backwards compatibility and job migrations applies somewhat for immediate scheduled jobs as well, for this we can just make sure that worker does graceful shutdown during upgrades and allows current running jobs to finish.

Copy link
Contributor

@begelundmuller begelundmuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pjain1 pjain1 merged commit 15d4688 into main Sep 19, 2024
10 checks passed
@pjain1 pjain1 deleted the payment_webhook branch September 19, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(billing): Billing failures or end of trial should hibernate project
2 participants